Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fade out unnecessary usings #2646

Closed
wants to merge 1 commit into from

Conversation

akshita31
Copy link
Contributor

@akshita31 akshita31 commented Nov 2, 2018

Related issue: #1179
#1245

Related API: microsoft/vscode#20219
https://code.visualstudio.com/updates/v1_25#_diagnostictag

Fade the unnecessary usings
usings

@codecov
Copy link

codecov bot commented Nov 2, 2018

Codecov Report

Merging #2646 into master will increase coverage by 0.13%.
The diff coverage is 50%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #2646      +/-   ##
=========================================
+ Coverage   65.27%   65.4%   +0.13%     
=========================================
  Files          99      99              
  Lines        4320    4371      +51     
  Branches      630     641      +11     
=========================================
+ Hits         2820    2859      +39     
- Misses       1313    1325      +12     
  Partials      187     187
Flag Coverage Δ
#integration 53.59% <50%> (+0.48%) ⬆️
#unit 84.38% <ø> (-0.45%) ⬇️
Impacted Files Coverage Δ
src/features/diagnosticsProvider.ts 78.52% <50%> (+1.81%) ⬆️
src/features/workspaceSymbolProvider.ts 41.46% <0%> (-19.26%) ⬇️
src/omnisharp/protocol.ts 87.95% <0%> (+2.45%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 425c7f9...63d538a. Read the comment docs.

@@ -325,7 +325,14 @@ class DiagnosticsProvider extends AbstractSupport {
private static _asDiagnostic(quickFix: protocol.QuickFix): vscode.Diagnostic {
let severity = DiagnosticsProvider._asDiagnosticSeverity(quickFix.LogLevel);
let message = `${quickFix.Text} [${quickFix.Projects.map(n => DiagnosticsProvider._asProjectLabel(n)).join(', ')}]`;
return new vscode.Diagnostic(toRange(quickFix), message, severity);
let diagnostic = new vscode.Diagnostic(toRange(quickFix), message, severity);
if(diagnostic.message.includes("Unnecessary using directive"))
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is kind of a hack. Roslyn has analyzers that will fade all sorts of unused things, so it might be nice to properly flow that information from omnisharp so we can fade everything...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes that sounds right. Will look into it.

@nitin88
Copy link

nitin88 commented Jan 20, 2019

Any update on this?

@savpek
Copy link
Contributor

savpek commented Feb 18, 2019

I played this feature little:

image

Theres Descriptor.CustomTags field on diagnostic that has value 'Unnecessary' when diagnostic should be faded out.

Updated omnishar-roslyn code (locally) with savpek/omnisharp-roslyn@a29348c

However CS0xxx diagnostics don't have that 'unnecessary' tag field, it is included only on roslyn analysis diagnostics (dunno why?). It seems very common that theres specific 'hidden' diagnostics with 'unnesessary' tag which only purpose is to fadeout parts of code. Like that IDE0035 in image.

In image theres hard coded CS0xxx codes to only fadeout items (CS0162, CS8019) and not show squiggle under text. They are unnecessary usings and unreachable code. I liked that look much better 😄 There may be better generic way to solve this, since there's IDE analysis that tells to hide those lines. However CS diagnostic adds squiggles since it don't have tags but i don't know how to solve that which IDE analysis is meant to hide certain CS analysis in which locations.

Without that bit hacky hardcoding certain CS diagnostics result looks like:
image

Where CS0162 adds squiggle and IDE035 fadeouts code.

I can link omnisharp-vscode side of code later this week here if needed used at this prototype.

@Ciberusps
Copy link

Updates?

@akshita31
Copy link
Contributor Author

@Ciberusps The relevant changes are already in OmniSharp and we will be working on consuming the new omnisharp version. It should be available soon.

@akshita31
Copy link
Contributor Author

I will close this PR in favor of #2873

@akshita31 akshita31 closed this Mar 20, 2019
@JoeRobich JoeRobich deleted the dev/akagarw/unnecessary_using_fade branch December 7, 2019 04:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants